feat(desktop): add Excel/spreadsheet viewer with diff support#1
Conversation
Add the ability to view .xlsx, .xls, .xlsm, .xlsb, and .ods files natively instead of showing "Binary file preview not supported". Features: - Full spreadsheet rendering with ExcelJS (formatting, borders, merged cells, fonts, colors, theme colors with tint, rich text, row heights, text wrapping, vertical text) - Print area aware column/row range detection - Auto fit-to-width column scaling - Multiple sheet tab navigation - Side-by-side diff viewer with cell-level change highlighting - Diff navigation (Prev/Next) with synchronized scrolling - Git binary file read via new readGitFileBinary tRPC procedure
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThis PR adds comprehensive spreadsheet file viewing and diffing support to the desktop application. It introduces a new tRPC endpoint for reading binary git blobs, Excel workbook parsing logic, dedicated UI components for viewing spreadsheets, and routing updates to display spreadsheets in specialized viewers instead of generic binary file messages. Changes
Sequence DiagramssequenceDiagram
participant User
participant UI as File Viewer
participant TRPC as tRPC Client
participant Server
participant Git as Git Repository
participant Parser as Workbook Parser
participant Renderer as Spreadsheet Renderer
User->>UI: View spreadsheet file
UI->>TRPC: readFile(filePath) query
TRPC->>Server: Fetch file data
Server->>Git: cat-file -p (read blob)
Git-->>Server: Binary blob
Server-->>TRPC: Base64-encoded content
TRPC-->>UI: File data received
UI->>Parser: parseWorkbook(base64Content)
Parser->>Parser: Decode & extract sheets, rows, cells
Parser->>Parser: Apply styling, merges, formulas
Parser-->>UI: ParsedSheet[]
UI->>Renderer: Render with sheets, column widths
Renderer->>Renderer: Calculate column widths
Renderer->>Renderer: Render rows with cells & styling
Renderer-->>User: Spreadsheet table display
sequenceDiagram
participant User
participant DiffUI as Spreadsheet Diff Viewer
participant Hook as useSpreadsheetDiff
participant TRPC as tRPC Client
participant Server
participant Parser as Workbook Parser
participant Differ as Diff Engine
participant Renderer as Diff Renderer
User->>DiffUI: View spreadsheet diff
DiffUI->>Hook: Load diff (workspaceId, filePath, diffCategory, commitHash)
Hook->>TRPC: Query original version (HEAD)
TRPC->>Server: Fetch blob at HEAD:path
Server-->>TRPC: Base64 content
Hook->>TRPC: Query modified version (working copy)
TRPC->>Server: Fetch file from worktree
Server-->>TRPC: Base64 content
Hook->>Parser: parseWorkbook(original base64)
Parser-->>Hook: ParsedSheet[] (original)
Hook->>Parser: parseWorkbook(modified base64)
Parser-->>Hook: ParsedSheet[] (modified)
Hook->>Differ: Align sheets, compute row/cell diffs
Differ->>Differ: Mark cells as added/removed/modified
Differ-->>Hook: DiffParsedSheet[]
Hook-->>DiffUI: Diff data ready
DiffUI->>Renderer: Render side-by-side tables with diff colors
Renderer->>Renderer: Color cells (red=removed, green=added, yellow=modified)
Renderer->>Renderer: Sync scroll positions between panes
Renderer-->>User: Synchronized diff display
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
#1 ダッシュボード戻り時のポーリング永続: activeWorkspaceId=null時に deactivateAll()を呼ぶ。setActiveWorkspace(null)対応追加 #2 deactivateされたWSのMapエントリ蓄積: 問題の根本はregisterが増え続ける ことではなく、全WSがisActive:trueで起動していたこと。#3で解消 #3 起動直後の全WSポーリング並走: registerWorkspaceをisActive:falseに変更。 タイマーはactivateWorkspace/setActiveWorkspaceでのみ起動 #4 WS切替時の初回表示5s遅延: activateWorkspace/setActiveWorkspace時に 即時sync(syncPRStatus+syncPRComments)を実行 追加改善: - startTimersに防衛的stopTimers追加(二重タイマー防止) - onWindowFocus()をデッドコードとして削除 - deactivateAll()メソッド追加
…sions #1 Target.closeTarget UI integrity v1/v2 secondary tab registry が webview "close" イベントを購読。 MCP の Target.closeTarget で Chromium が webContents を破棄すると guest 側が close を発火し、registry が closeTab → unregisterTab 経由で paneTabTargetIds を整理。tab バー UI と CDP allowedTargetIds が同期した状態を保つ。 #2 target="_blank" / window.open を MCP 可視に windowOpenHandler の非 new-window 分岐で new-window event を emit していた箇所を create-tab-requested:${paneId} に置換。同じペイン 内の secondary tab として生成されるので paneTabTargetIds に入り、 MCP が list_pages / select_page で扱える。Chrome の target="_blank" デフォルト (新タブ) 挙動に揃う。split-pane / workspace-tab が 欲しいケースは既存の "Open in Split" コンテキストメニューでカバー。 #3 非 media 権限の UI prompt 化 SITE_PERMISSION_KINDS に geolocation / notifications / clipboard-read を追加。browser-site-permission-manager が Electron の setPermissionRequestHandler で media 同様の consent flow に乗せる。 既存の permissionRequested イベント経路はそのまま再利用。 認識しない permission は従来通り permissive で許可。
) * feat(desktop): workspace pane store registry (v2 PR 3) PR 3 of the canonical workspace.create() refactor — see plans/20260430-pane-store-registry-pr3.md. Decouples pane state ownership from the workspace route mount. The V2 workspace pane store now lives in a module-level registry keyed by workspaceId, rather than being created inside `useV2WorkspacePaneLayout` on mount. This unblocks PR 4 (`workspace.create()`) returning a list of already-started sessions that the renderer can write directly into the store before — or without — navigating. Files - New: apps/desktop/src/renderer/lib/workspace-pane-registry/ - workspace-pane-registry.ts: initWorkspacePaneRegistry(deps), getOrCreateWorkspacePaneStore(workspaceId), dropWorkspacePaneStore, test reset helper. Bidirectional sync wired at store creation: seeds from v2WorkspaceLocalState.paneLayout, writes back on store changes (when row exists), pushes external row changes into store, snapshot guard prevents echo. - addLaunchPanes.ts: takes the doc-shape launches array, dedupes by terminalId / chatSessionId, focuses. Attach-only — no initialCommand. Existing presets / pending-launch path keep using addTab directly. - 14 unit tests against an in-memory localStorage-backed collection. - Modified: useV2WorkspacePaneLayout — drops 87 lines of useState + useLiveQuery + persistence effects, replaced by a single getOrCreateWorkspacePaneStore call. ensureWorkspaceInSidebar still runs on mount. - Modified: CollectionsProvider — initializes the registry inside the collections useMemo (synchronously, before children render) so the workspace route's useState initializer sees a ready registry. Reinits on org switch. Out of scope (deferred) - The pending-row launch-adoption path (useConsumePendingLaunch) is unchanged. PR 5 deletes it when the new workspace modal moves onto workspace.create(). - TerminalPaneData.initialCommand is already optional in current code and TerminalPane already handles undefined; no change needed. * fix(panes,desktop): two regressions found while testing PR3 Manual testing of the modal → new workspace → agent flow on PR3 turned up two bugs that combined to wipe the agent's `initialCommand` before the terminal pane could send it. panes/Tab.tsx — add `key={pane.id}` to <Pane>: - Without a key, React reconciles Pane instances by position. Switching the active tab to one whose Pane occupies the same layout slot reuses the previous Pane's component instance. Hooks (notably `useRef(paneData.initialCommand)` in TerminalPane) keep stale values from the prior pane, so the agent's initialCommand never reached terminalRuntimeRegistry.connect. - Latent bug — didn't manifest before PR3 because the legacy useV2WorkspacePaneLayout's effect-based "load row → replaceState" timing happened to give Pane a fresh mount each time. PR3's registry seeds the store before the route mounts, exposing the issue. workspace-pane-registry.ts — strengthen the row→store guard: - Compare incoming row state to the *current* store snapshot, not the tracked `lastSyncedSnapshot`. Two reasons: 1. Tanstack DB delivers existing rows as initial-state `insert` events on subscribe, with the post-our-write value. The old guard mistakenly thought these were unrelated external updates. 2. Immer's `update(draft => { draft.paneLayout = ... })` writes do not preserve key insertion order, so JSON.stringify produced different strings for structurally equal store/row states. - Also introduce `deepSortKeys`-based stable serialization so snapshot equality is structural rather than sensitive to key order. Validated end-to-end: modal-create → agent terminal opens → "claude …" prompt runs and Claude responds. 14/14 unit tests still pass. * fix(desktop): drop pane store on workspace deletion Calls dropWorkspacePaneStore(workspaceId) from useDashboardSidebarState.removeWorkspaceFromSidebar so registry entries don't accumulate forever when workspaces are removed. Pairs with the existing v2WorkspaceLocalState.delete + cleanupWorkspacePaneRuntimes calls in the same function. * fix(desktop): address PR review on pane-store registry Three valid review comments on the registry: 1. Idempotency check compared the wrapper object reference, not the collection instance (greptile P2). With the wrapper-only check, every call site passing a fresh `{ v2WorkspaceLocalState: ... }` literal would have torn down all stores even when the underlying collection hadn't changed. Switched to instance-level comparison; passing the same collection instance is now a true no-op. 2. Pre-mount addLaunchPanes panes weren't reliably persisted (greptile P2 + cubic P1). When `addLaunchPanes` runs before the v2WorkspaceLocal State row exists, the store's write-back is skipped (no row), then `ensureWorkspaceInSidebar` later inserts the row with EMPTY paneLayout. The previous code's "compare incoming row to current store" guard would have called `replaceState(EMPTY)` and wiped the in-memory tabs. New logic uses three-way reconciliation: compare incoming, store, and the last-synced agreement to decide direction. If the store has unsynced mutations vs lastSyncedSnapshot, push store→row; otherwise pull row→store. Added a regression test. 3. `__resetWorkspacePaneRegistryForTests` removed from the public barrel (greptile P2). Tests now import it directly from the implementation file. The useMemo-as-side-effect concern (cubic P2 + greptile P2) is intentional for the synchrony requirement (see expanded comment in CollectionsProvider). Mitigation: #1 above ensures any unintended memo recomputation receiving the same collection is a true no-op. * chore(desktop): tighten pane-registry comments - addLaunchPanes JSDoc: remove the now-stale 'persists when the next store change writes back' line. The registry's three-way reconciliation persists pre-mount panes the moment the row is inserted; no follow-up mutation needed. - Test-only helper imports: standardize the explanatory comment to one short line in both test files. * chore: format two terminal-key-event-handler files (CI unblocker) Auto-fixable lint issues introduced in superset-sh#3968 (terminal key handler share between v1/v2). Not related to this PR's pane-registry work, but blocks CI; folding the trivial reformat into the rebase here so PR3 can land. Should be a no-op once main resolves it.
…uperset-sh#3999) * perf(workspace-fs): cap searchIndexCache + pathTypes for worktree scaling Both maps previously had no eviction and grew monotonically with worktree count (searchIndexCache) and file event count (pathTypes). Adds LRU(12) + 30-min idle TTL to searchIndexCache and LRU(10k) cap to per-watcher pathTypes. Eviction in pathTypes loses only the directory-type hint; the next event for that path falls back to stat() (existing slow path). Measured (cache-and-paths-memory.bench.test.ts): - searchIndexCache @ 130 worktrees: 6.87 MB → 2.02 MB (-71%) - pathTypes @ 20k unique paths: 8.69 MB → 2.54 MB (-71%) - searchIndexCache cap holds at 12 entries, pathTypes at 10000 Adds findings audit + fix plan + reproduction tests + benchmarks for the broader v2 worktree-perf investigation. Notably, the host-service syncWorkspaceBranches 30s polling (1542 ms/tick at N=20 worktrees, real git subprocesses) is documented and reproduced but not fixed in this commit; follow-up PR will subscribe the pull-requests runtime to GitWatcher.onChanged. See plans/v2-paths-worktree-perf-findings.md for the full audit and plans/v2-paths-worktree-perf-fix-plan.md for the remaining work. * docs: expand fix plan with handoff checklist + Fix #1 implementation notes Adds concrete pickup steps, app.ts wiring order, concurrency notes, and a mapping of which existing tests/benchmarks change vs stay. The next session (or fresh agent) implementing Fix #1 should be able to read the plan top-to-bottom and execute without re-deriving context. * perf(host-service): event-driven pull-requests sync via GitWatcher Replaces the unconditional 30s `syncWorkspaceBranches` polling timer with a `GitWatcher.onChanged` subscription so idle worktrees cost ~0 git subprocesses regardless of N. Branch / HEAD / upstream changes are picked up at ~430 ms p50 (was up to 30 s). Lift `GitWatcher` to a standalone instance in `app.ts` so both `EventBus` (broadcasts to clients) and `PullRequestRuntimeManager` (event-driven branch sync) share one watcher. `syncWorkspaceBranches` becomes the safety-net sweep: still O(N) per call, but cadence drops from 30 s → 5 min. Project-level PR refresh interval also drops from 20 s → 5 min — branch changes drive their own `refreshProject` via `syncOneWorkspace`, so the polling is only there to catch external PR opens. Concurrency stays safe via the existing `inFlightProjects` guard. Workspace deletion races no-op cleanly via a fresh row lookup in `syncOneWorkspace`. Tests updated: - Existing scaling unit + integration tests now describe the safety-net sweep (still pin the O(N) per-call shape). - New integration test wires a real `GitWatcher` + `WorkspaceFilesystemManager` and asserts a `git commit` in 1/5 worktrees triggers exactly 4 git ops on 1 worktree. - Bench replaces "ms per polling tick" with event-to-DB-update latency (427 ms measured) plus the long-cadence safety-net sweep cost. Closes Fix #1 + Fix #4 in plans/v2-paths-worktree-perf-fix-plan.md. * fix(perf): address PR review — TTL on cache hit, sync serialization, test timeout - search.ts: enforce idle TTL on cache hits — previously a hot key was bumped forever and only sibling-key misses ran the TTL sweep, so a 30+ min idle entry would still be served stale on next access. Now the hit path checks freshness and rebuilds when expired. - pull-requests.ts: serialize syncOneWorkspace per workspaceId via Map<workspaceId, Promise>. GitWatcher only debounces 300 ms; two bursts far enough apart could run concurrent git reads and let the slower write clobber the newer snapshot. - pull-requests-scaling.integration.test.ts: fix `timeout` → `timeoutMs` in two waitFor calls. The wrong key was silently dropped, falling back to the 5 s default and risking flake on slow CI. * fix(perf): coalesce per-workspace sync — running + rerun-pending flag Replaces the linear promise chain with a "running + rerun pending" flag so N events for the same workspace collapse into at most one running sync + one queued rerun. Since each sync reads fresh state, queuing additional redundant syncs adds no value — it just wastes git subprocesses. Bounded under sustained watcher noise (long interactive rebase, bulk ref churn), where the previous chain could pile up dozens of sequential no-op syncs. * fix(workspace-fs): exempt directories from pathTypes LRU + de-flake cap test Splits WatcherState.pathTypes into filePaths (LRU-capped at 10k) and directoryPaths (uncapped Set). Pre-fix, the unified Map could LRU-evict a directory hint, after which a delete event for that directory fell back to isDirectory=false and patchSearchIndexesForRoot only pruned the exact path — leaving descendant search-index entries stale until the next full rebuild. Directory count per worktree is bounded by repo structure (O(100s) even for huge repos), so tracking them uncapped is fine; only the file-path stream grows unboundedly. Also fixes the cap-eviction test which was polling on a 95% event-count threshold (10_000 cap × 95% = 9,690 events, which can land before eviction triggers and stall under coalesced delivery). Now polls on the actual eviction outcome — `pathTypes.has(cap-0.tmp) === false` — and asserts cap on filePaths.size directly via a new getFilePathsSize helper. Bench predicate is similarly capped at min(target, FILE_PATHS_MAX) to avoid spinning the deadline once size plateaus. * fix(perf): route safety-net sweep through workspaceSyncState queue The serialization queue added previously only covered the watcher-driven path; `syncWorkspaceBranches` (initial startup sweep + 5-min safety net) still called `syncWorkspaceRow` directly, so it could race a concurrent watcher-triggered sync for the same workspace and clobber newer state. The sweep now iterates ids and routes each through enqueueWorkspaceSync, which coalesces — if a watcher sync is already running for a workspace, the sweep just flips rerunPending and awaits the running promise. Sequential per-workspace iteration matches the original sweep's git-subprocess concurrency profile. Test mocks override syncOneWorkspace to bypass the drizzle .where() chain, since the sweep now performs a per-workspace row lookup that doesn't compose cleanly with the existing chained mock structure. * fix(perf): address remaining PR review nits - watch.ts: serialize normalizeEvent calls in flushPendingEvents — replaces Promise.all with a sequential for-of loop so LRU mutations land in event order, not stat-completion order. Net code roughly unchanged but removes the concurrency hazard coderabbit flagged. - watch-pathtypes-growth.test.ts: consolidate manager cleanup into afterEach. Tests register managers via createManager() and stop calling unsubscribe() + manager.close() inline. afterEach closes them all even if a test throws. Net code reduction (-16 lines). - pull-requests.test.ts: tighten warn assertion to match the actual "Failed to sync workspace" prefix instead of accepting any console.warn. - v2-paths-worktree-perf-fix-plan.md: align acceptance criteria wording with the tests that actually landed. * refactor(workspace-fs): drop dead TTL sweep + inline one-shot bump helper `evictStaleSearchIndexEntries` is redundant: per-hit TTL check on getSearchIndex line 299-307 already discards stale entries on access, and the hard LRU cap of 12 bounds memory regardless of TTL behavior. The build-path sweep over all entries was duplicated work that did nothing the LRU eviction wasn't already doing. `bumpAndReturnCachedIndex` had one caller and was 4 lines of body — inlined directly into the hit path. Net -23 lines. * fix(ci): remove typecheck shim, exclude benches from default test run CI typecheck failed because workspace-fs had a hand-rolled src/bun-test.d.ts shim with a minimal `expect` (only `toContain` / `toEqual` / `toHaveLength` / `toBeNull` / `toBeTruthy`) that shadowed the real bun-types definitions. Adding bun-types as a devDependency and dropping the shim restores the full matcher surface. CI tests OOM'd on @superset/workspace-fs#test (exit 137). The cache-and-paths-memory bench creates 130 worktrees × 200 files + heap snapshots and was being picked up by default `bun test` because of its `.bench.test.ts` suffix. Renamed both bench files to `.bench.ts` (off the auto-discovery pattern) and added explicit `bun run bench` scripts so they're still runnable on demand. Also tightened search-cache-eviction.test.ts array typings: previous `unknown[]` was fine under the shim's permissive `expect` but doesn't typecheck against the real signature. Now uses `Awaited<ReturnType<typeof getSearchIndex>>[]` with explicit guards for noUncheckedIndexedAccess. * fix(ci): slim integration test — drop scaling cases covered by mock units CI host-service#test was OOMing (exit 137) because the integration test created 4 scenarios with simple-git + WorkspaceFilesystemManager + GitWatcher per scenario (15 worktrees + 15 parcel-watcher subscriptions total). Two of those scenarios just re-asserted what the mock-based unit test in test/pull-requests-scaling.test.ts already pins — linearity of git-subprocess count and "safety-net walks all N". Removes the duplicative integration scaling cases. Keeps only the event-driven scenario, which is the unique integration coverage (verifies a real `git commit` in one workspace triggers exactly one single-workspace sync, with the others staying quiet). Reduced from 5 to 3 worktrees — enough to prove "only the target was touched". Net: -142 lines, ~80% fewer worktrees spawned per test file run.
Summary
readGitFileBinarytRPCプロシージャを追加Changes
New files (6)
SpreadsheetViewer/— Excel表示コンポーネント群parseWorkbook.ts— ExcelJSパースロジック(書式、罫線、テーマカラー、結合セル対応)SpreadsheetViewer.tsx— 通常表示コンポーネントSpreadsheetDiffViewer.tsx— サイドバイサイドdiff表示useSpreadsheetData.ts— ファイル読み込み+パースhookuseSpreadsheetDiff.ts— diff用データ取得+比較hookindex.ts— barrel exportModified files (7)
file-types.ts—isSpreadsheetFile()追加 (+10行)file-contents.ts—readGitFileBinarytRPCプロシージャ追加 (+49行)FileViewerContent.tsx— バイナリ/diff分岐にスプレッドシート判定追加 (+37行)FileViewerPane.tsx— 新props受け渡し (+4行)WorkspaceFilePreviewContent.tsx— V2プレビューにスプレッドシート分岐 (+10行)package.json— exceljs依存追加bun.lock— lockfile更新Test plan
Summary by CodeRabbit
Release Notes